Merged
Conversation
1. Fixed _interpolate_charge_state_segmented Bug (Critical) File: flixopt/transform_accessor.py:1965-2043 The function was incorrectly decoding timestep_mapping using timesteps_per_cluster. For segmented systems, timestep_mapping encodes cluster * n_segments + segment_idx, so this produced wrong cluster indices. Fix: Compute original period index and position directly from timestep indices instead of decoding from timestep_mapping. 2. Implemented EXPAND_FIRST_TIMESTEP for Segmented Systems Only File: flixopt/transform_accessor.py:2045-2100 Added _expand_first_timestep_only() method that places startup/shutdown events at the first timestep of each segment (not cluster). Key design decisions: - Segmented systems: Events placed at first timestep of each segment (timing within segment is lost) - Non-segmented systems: Normal expansion preserves timing within cluster (no special handling needed) 3. Added Tests - test_startup_shutdown_first_timestep_only: Verifies segmented systems place events at segment boundaries - test_startup_timing_preserved_non_segmented: Verifies non-segmented systems preserve timing within clusters 4. Updated Docstrings Clarified in expand() docstring that: - Binary events in segmented systems go to first timestep of each segment - Non-segmented systems preserve timing via normal expansion
1. Fixed _interpolate_charge_state_segmented Bug (Critical) File: flixopt/transform_accessor.py:1965-2043 The function was incorrectly decoding timestep_mapping using timesteps_per_cluster. For segmented systems, timestep_mapping encodes cluster * n_segments + segment_idx, so this produced wrong cluster indices. Fix: Compute original period index and position directly from timestep indices instead of decoding from timestep_mapping. 2. Implemented EXPAND_FIRST_TIMESTEP for Segmented Systems Only File: flixopt/transform_accessor.py:2045-2100 Added _expand_first_timestep_only() method that places startup/shutdown events at the first timestep of each segment (not cluster). Key design decisions: - Segmented systems: Events placed at first timestep of each segment (timing within segment is lost) - Non-segmented systems: Normal expansion preserves timing within cluster (no special handling needed) 3. Added Tests - test_startup_shutdown_first_timestep_only: Verifies segmented systems place events at segment boundaries - test_startup_timing_preserved_non_segmented: Verifies non-segmented systems preserve timing within clusters 4. Updated Docstrings Clarified in expand() docstring that: - Binary events in segmented systems go to first timestep of each segment - Non-segmented systems preserve timing via normal expansion
…scenario) dimensions: 1. For each missing (period_label, scenario_label) key, it selects the specific period/scenario slice from the original variable using .sel(..., drop=True) 2. Then it slices and reshapes that specific slice 3. All slices in filled_slices now have consistent dimensions ['cluster', 'time'] + other_dims without 'period' or 'scenario' coordinates This ensures all DataArrays being concatenated have the same structure. Can you try running your clustering code again?
New Modules Created
1. flixopt/clustering/iteration.py - Shared iteration infrastructure:
- DimSliceContext dataclass for (period, scenario) slice context
- DimInfo dataclass for dimension metadata
- iter_dim_slices() generator for standardized iteration
- iter_dim_slices_simple() convenience function
2. flixopt/clustering/aggregation.py - Aggregation helpers:
- combine_slices_to_dataarray() - Unified slice combination function
- build_typical_dataarrays() - Build typical periods DataArrays
- build_segment_durations() - Build segment duration DataArray
- build_cluster_weights() - Build cluster weight DataArray
- build_clustering_metrics() - Build metrics Dataset
- calculate_clustering_weights() - Calculate clustering weights from data
- build_cluster_config_with_weights() - Merge weights into ClusterConfig
- accuracy_to_dataframe() - Convert tsam AccuracyMetrics to DataFrame
3. flixopt/clustering/expansion.py - Expansion helpers:
- VariableExpansionHandler class - Handles variable-specific expansion logic
- interpolate_charge_state_segmented() - Interpolate state variables in segments
- expand_first_timestep_only() - Expand binary events to first timestep
- build_segment_total_varnames() - Build segment total variable names
- append_final_state() - Append final state value to expanded data
4. Updated flixopt/clustering/intercluster_helpers.py:
- Added combine_intercluster_charge_states() - Combine charge_state with SOC_boundary
- Added apply_soc_decay() - Apply self-discharge decay to SOC values
Refactored flixopt/transform_accessor.py
- cluster() method: Now uses DimInfo and iter_dim_slices() for standardized iteration
- expand() method: Uses VariableExpansionHandler and extracted helper functions
- _build_reduced_flow_system(): Signature changed to use DimInfo instead of separate periods/scenarios lists
- _build_reduced_dataset(): Updated to use DimInfo and combine_slices_to_dataarray()
- apply_clustering(): Uses DimInfo for cleaner key conversion
Removed from transform_accessor.py
- _calculate_clustering_weights() → moved to clustering/aggregation.py
- _build_cluster_config_with_weights() → moved to clustering/aggregation.py
- _accuracy_to_dataframe() → moved to clustering/aggregation.py
- _build_cluster_weight_da() → moved to clustering/aggregation.py
- _build_typical_das() → moved to clustering/aggregation.py
- _build_segment_durations_da() → moved to clustering/aggregation.py
- _build_clustering_metrics() → moved to clustering/aggregation.py
- _combine_slices_to_dataarray_generic() → replaced by unified combine_slices_to_dataarray()
- _combine_slices_to_dataarray_2d() → replaced by unified combine_slices_to_dataarray()
- _combine_intercluster_charge_states() → moved to clustering/intercluster_helpers.py
- _apply_soc_decay() → moved to clustering/intercluster_helpers.py
- _build_segment_total_varnames() → moved to clustering/expansion.py
- _interpolate_charge_state_segmented() → moved to clustering/expansion.py
- _expand_first_timestep_only() → moved to clustering/expansion.py
Updated flixopt/clustering/__init__.py
- Exports all new helpers from the new modules
Tests
- All 102 tests in test_cluster_reduce_expand.py pass
- All 43 tests in test_clustering_io.py pass
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughModularizes clustering, expansion, and inter-cluster operations by extracting implementations from transform_accessor into dedicated clustering submodules; adds DimInfo for multi-dimensional (period/scenario) handling; re-exports new public APIs via flixopt.clustering.init; updates consumers to use the new helpers. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
participant TA as transform_accessor
participant DIM as clustering.iteration::DimInfo / iter_dim_slices
participant TSAM as tsam.aggregate
participant AGG as clustering.aggregation
participant EXP as clustering.expansion::VariableExpansionHandler
participant ICC as clustering.intercluster_helpers
TA->>DIM: build DimInfo from flow_system
TA->>DIM: iterate slices via iter_dim_slices
loop per (period,scenario)
TA->>TSAM: call aggregate on selected data
TSAM-->>TA: Aggregation results (per-slice)
TA->>AGG: combine_slices_to_dataarray / build_typical_dataarrays
AGG-->>TA: reduced DataArrays/Datasets
end
TA->>EXP: decide expansion paths via VariableExpansionHandler
EXP-->>TA: expanded per-variable DataArrays
TA->>ICC: combine_intercluster_charge_states (apply_soc_decay if needed)
ICC-->>TA: SOC-reconstructed solution
TA->>TA: assemble final expanded FlowSystem / dataset
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
flixopt/transform_accessor.py (2)
376-387: Update_build_cluster_assignments_dato use the unifiedcombine_slices_to_dataarrayfunction.The method calls
self._combine_slices_to_dataarray_generic(), which no longer exists—it was unified intocombine_slices_to_dataarray()(see comment at line 1371-1372). This will raiseAttributeErrorwhen invoked.Proposed fix
- return self._combine_slices_to_dataarray_generic( - cluster_assignments_slices, ['original_cluster'], periods, scenarios, 'cluster_assignments' - ) + dim_info = DimInfo(periods=periods, scenarios=scenarios) + return combine_slices_to_dataarray( + slices=cluster_assignments_slices, + dim_info=dim_info, + base_dims=['original_cluster'], + name='cluster_assignments', + )
1320-1351: Add explicit period/scenario validation before callingclustering.results.apply().The docstring promises a
ValueErrorif clustering dimensions don't match, but currently validation is implicit via xarray's.sel(), which raisesKeyErrorwith an unclear message. Add a preemptive guard that validatesclustering.coords(e.g., checking that all periods/scenarios inclusteringexist in the dataset) before line 1340, with a clear error message matching the docstring's contract.
🤖 Fix all issues with AI agents
In `@flixopt/clustering/intercluster_helpers.py`:
- Around line 277-335: The code assumes loss_value has a .values attribute which
fails for scalar returns from _scalar_safe_reduce; update apply_soc_decay to
handle both scalars and DataArray: after loss_value = _scalar_safe_reduce(...),
normalize it by wrapping scalars into an xarray/DataArray or use numpy
conversion (e.g., loss_arr = np.asarray(loss_value)) and replace the check if
not np.any(loss_value.values > 0) with if not np.any(loss_arr > 0); then use
loss_arr (or xr.DataArray(loss_value) if you prefer) for the decay calculation
(decay_da = (1 - loss_arr) ** time_within_period_da) so both scalar floats and
DataArrays from storage.relative_loss_per_hour work without AttributeError.
In `@flixopt/transform_accessor.py`:
- Around line 1164-1176: The current check uses is_multi_dimensional =
dim_info.has_periods or dim_info.has_scenarios which wrongly blocks
extremes.method 'new_cluster'/'append' even for a single period/scenario slice;
change the guard to detect multiple period/scenario combinations instead (e.g.,
compute total_slices from dim_info — using available attributes like
dim_info.periods or dim_info.n_periods and dim_info.scenarios or
dim_info.n_scenarios — and treat multi-dimensional only when total_slices > 1),
then only raise the ValueError for extremes.method in ('new_cluster', 'append')
when total_slices > 1. Ensure you reference the same variables used in the diff
(dim_info, extremes, extreme_method) when updating the condition.
🧹 Nitpick comments (3)
flixopt/clustering/aggregation.py (1)
98-100: Preserve fullbase_dimsordering after concatenation.Only the first base dimension is forced to the front, so other base dims (e.g.,
time) can end up afterperiod/scenario, contradicting the stated order. Consider transposing with all base dims up front.♻️ Proposed change
- result = result.transpose(base_dims[0], ...) + result = result.transpose(*base_dims, ...)tests/test_cluster_reduce_expand.py (2)
841-870: Align thenew_clusterexpectation with the assertion.The description says
new_clustershould increasen_clusters, but the assertion allows no increase. Consider tightening the assertion or updating the wording to match the intended behavior.💡 Possible tweak
- assert fs_clustered.clustering.n_clusters >= 2 + assert fs_clustered.clustering.n_clusters > 2
908-923: Test name/docstring doesn’t match the ExtremeConfig method used.The test is named for
new_cluster, but it configuresmethod='append'. Consider renaming the test/docstring or switching the method to keep intent unambiguous.💡 Possible rename
- def test_extremes_new_cluster_with_segments(self, solver_fixture, timesteps_8_days): - """Test that method='new_cluster' works correctly with segmentation.""" + def test_extremes_append_with_segments(self, solver_fixture, timesteps_8_days): + """Test that method='append' works correctly with segmentation."""
… to handle both scalar and DataArray returns from _scalar_safe_reduce using np.asarray().
2. flixopt/transform_accessor.py: Fixed the multi-dimensional check to only block extremes.method='new_cluster'/'append' when there are actually multiple slices (total_slices
> 1), not just when periods or scenarios exist.
3. flixopt/clustering/aggregation.py: Fixed combine_slices_to_dataarray to transpose with all base dims in order (*base_dims, ...) instead of just the first one.
4. tests/test_cluster_reduce_expand.py:
- Updated test_extremes_new_cluster_increases_n_clusters docstring to clarify that tsam may or may not add clusters (the >= 2 assertion was actually correct)
- Renamed test_extremes_new_cluster_with_segments to test_extremes_append_with_segments and updated docstring to match the actual method='append' used
…m-rework # Conflicts: # flixopt/transform_accessor.py # tests/test_cluster_reduce_expand.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
New Features
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.